Fix #16944: Invalid filtering of IPv6 with FILTER_FLAG_NO_RES_RANGE - #17111
Fix #16944: Invalid filtering of IPv6 with FILTER_FLAG_NO_RES_RANGE#17111derickr wants to merge 10 commits into
Conversation
|
/cc @nielsdos @nicolas-grekas — It makes most sense to review this commit-by-commit. |
1096e89 to
0a5aab6
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
This is much much nicer, thanks a lot!
There was a problem hiding this comment.
The errata for RFC6890 changes this, see https://www.rfc-editor.org/errata_search.php?rfc=6890&rec_status=1&presentation=records
Should instead be: /* RFC 0791 - This network */.
There was a problem hiding this comment.
It actually splits up the record into two, so I've made this change instead:
}
/* }}} */
-/* From the tables in RFC 6890 - Special-Purpose IP Address Registries */
-static bool ipv4_get_status_flags(int ip[8], bool *global, bool *reserved, bool *private)
+/* From the tables in RFC 6890 - Special-Purpose IP Address Registriesi
+ * Including errata: https://www.rfc-editor.org/errata_search.php?rfc=6890&rec_status=1 */
+static bool ipv4_get_status_flags(const int ip[8], bool *global, bool *reserved, bool *private)
{
*global = false;
*reserved = false;
*private = false;
if (ip[0] == 0) {
+ /* RFC 0791 - This network */
+ *reserved = true;
+ } else if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0) {
/* RFC 1122 - This host on this network */
*reserved = true;
} else if (ip[0] == 10) {
There was a problem hiding this comment.
Seems good to me. The compiler will optimize that anyway so it's okay to be verbose 🙂
There was a problem hiding this comment.
The range is 192.0.0.0/29. So this seems wrong because it is 192.0.0.0 until 192.0.0.7, because only the 3 bottom bits are free to use. So both the range and indices of the ip in this check are not correct.
There was a problem hiding this comment.
Thanks! It looks like the original code didn't even check for this one.
There was a problem hiding this comment.
Could be const int ip[8] in theory, but doesn't matter much.
|
Also note that this should target branch PHP-8.3, see https://externals.io/message/125995 |
derickr
left a comment
derickr
left a comment
There was a problem hiding this comment.
Also going to retarget this for PHP 8.3
There was a problem hiding this comment.
Thanks! It looks like the original code didn't even check for this one.
There was a problem hiding this comment.
It actually splits up the record into two, so I've made this change instead:
}
/* }}} */
-/* From the tables in RFC 6890 - Special-Purpose IP Address Registries */
-static bool ipv4_get_status_flags(int ip[8], bool *global, bool *reserved, bool *private)
+/* From the tables in RFC 6890 - Special-Purpose IP Address Registriesi
+ * Including errata: https://www.rfc-editor.org/errata_search.php?rfc=6890&rec_status=1 */
+static bool ipv4_get_status_flags(const int ip[8], bool *global, bool *reserved, bool *private)
{
*global = false;
*reserved = false;
*private = false;
if (ip[0] == 0) {
+ /* RFC 0791 - This network */
+ *reserved = true;
+ } else if (ip[0] == 0 && ip[1] == 0 && ip[2] == 0 && ip[3] == 0) {
/* RFC 1122 - This host on this network */
*reserved = true;
} else if (ip[0] == 10) {
There was a problem hiding this comment.
I did another pass over the PR, and I only have one question for this line here.
RFC8190 updates some parts of RFC6890, but the only change relevant to this RP seems to be "2.2. Updates to the IPv4 Special-Purpose Address Registry". It states:
Limited Broadcast prefix (255.255.255.255/32) - The Reserved-by-
Protocol value has changed from False to True. This change was
made to align the registry with reservation of the limited
broadcast address with Section 7 of [RFC919].
So this likely now needs to set reserved to true.
…turned to IANA (RFC 3701)
103c9ae to
50f99af
Compare
|
I have merged this after a rebase, manually, as GitHub got confused about having changed branches for the PR. |
2 participants
It also refactors the code to use the actual tables from RFC 6890, and fixed other bugs with these ranges that were introduced with #7893